Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do includes/libraries based more off a traditional installation #56

Merged
merged 5 commits into from
Dec 20, 2024
Merged

Do includes/libraries based more off a traditional installation #56

merged 5 commits into from
Dec 20, 2024

Conversation

lindsayad
Copy link
Contributor

@nmnobre nmnobre self-requested a review November 26, 2024 13:11
@makeclean
Copy link

This one seems pretty non contentious - surprised the code wasn't already like this - maybe part of a speedrun

@alexanderianblair
Copy link
Contributor

Yup - this looks good - once MFEM miniapp headers are properly installed (following MFEM #4583 being merged) we can merge this. @nmnobre is well placed to review here, since he's reviewing your PR on the MFEM side too!

@nmnobre nmnobre mentioned this pull request Dec 2, 2024
Merged
@nmnobre nmnobre force-pushed the leverage-actual-mfem-installation branch 4 times, most recently from 402ec23 to 2270f34 Compare December 19, 2024 17:37
lindsayad
lindsayad commented Dec 19, 2024
View reviewed changes
include/problem/MFEMProblem.h Outdated Show resolved Hide resolved
include/problem/MFEMProblem.h Outdated Show resolved Hide resolved
include/problem/MFEMProblem.h Outdated Show resolved Hide resolved
include/problem_operators/problem_operator.h Outdated Show resolved Hide resolved
include/problem_operators/time_domain_problem_operator.h Outdated Show resolved Hide resolved
@nmnobre nmnobre force-pushed the leverage-actual-mfem-installation branch 2 times, most recently from afbe9bb to a7d8287 Compare December 19, 2024 21:11
@nmnobre nmnobre marked this pull request as ready for review December 19, 2024 22:51
@nmnobre nmnobre marked this pull request as draft December 19, 2024 22:55
@nmnobre nmnobre force-pushed the leverage-actual-mfem-installation branch from e4bae01 to a8e3fb1 Compare December 19, 2024 23:13
nmnobre
nmnobre approved these changes Dec 20, 2024
View reviewed changes
Copy link
Collaborator

@nmnobre nmnobre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this, thanks @lindsayad! :)

I guess if we want to merge this cleanly, we should first merge the changes to the Dockerfile, build and upload the image, at which point the tests here should then pass without any hacks. What do you think @alexanderianblair?

@alexanderianblair
Copy link
Contributor

I'm happy with this, thanks @lindsayad! :)

I guess if we want to merge this cleanly, we should first merge the changes to the Dockerfile, build and upload the image, at which point the tests here should then pass without any hacks. What do you think @alexanderianblair?

Yes, this would be my preference here

@nmnobre nmnobre mentioned this pull request Dec 20, 2024
Merged
@nmnobre nmnobre force-pushed the leverage-actual-mfem-installation branch from a8e3fb1 to a31c2d4 Compare December 20, 2024 12:34
@codecov Codecov
Copy link

codecov bot commented Dec 20, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/bcs/MFEMBoundaryCondition.C 100.00% <ø> (ø)
unit/src/MFEMEssentialBCTest.C 100.00% <ø> (ø)

... and 16 files with indirect coverage changes

@nmnobre nmnobre marked this pull request as ready for review December 20, 2024 14:06
@alexanderianblair alexanderianblair merged commit 2de76d6 into aurora-multiphysics:main Dec 20, 2024
3 checks passed
@lindsayad lindsayad deleted the leverage-actual-mfem-installation branch December 23, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmnobre nmnobre nmnobre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lindsayad @makeclean @alexanderianblair @nmnobre